-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test against openff-interchange-0.4.0beta1
#1930
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There's a packaging issue uncovered only by my need/strong preference to use the most recent version of the toolkit error libmamba Could not solve for environment specs
The following packages are incompatible
├─ openff-interchange-base >=0.4.0beta1 is installable and it requires
│ └─ openff-toolkit-base ~=0.16.4 , which requires
│ └─ zstandard 0.18 , which can be installed;
├─ python 3.12** is installable with the potential options
│ ├─ python [3.12.0|3.12.1|...|3.12.5] would require
│ │ └─ python_abi 3.12.* *_cp312, which can be installed;
│ └─ python [3.12.0rc3|3.13.0rc1] would require
│ └─ _python_rc, which does not exist (perhaps a missing channel);
└─ qcportal >=0.[50](https://github.com/openforcefield/openff-toolkit/actions/runs/10685152619/job/29617269321#step:4:53) is not installable because it requires
└─ zstandard but there are no viable options
├─ zstandard [0.10.2|0.11.0|...|0.8.1] would require
│ └─ python [2.7* |>=2.7,<2.8.0a0 ], which conflicts with any installable versions previously reported;
├─ zstandard [0.10.2|0.11.0|0.11.1|0.12.0|0.13.0] would require
│ └─ python >=3.6,<3.7.0a0 , which conflicts with any installable versions previously reported;
├─ zstandard [0.10.2|0.11.0|0.11.1|0.12.0|0.13.0] would require
│ └─ python >=3.7,<3.8.0a0 , which conflicts with any installable versions previously reported;
├─ zstandard [0.12.0|0.13.0] would require
│ └─ python >=3.8,<3.9.0a0 , which conflicts with any installable versions previously reported;
├─ zstandard [0.14.0|0.14.1|0.15.1|0.15.2] would require
│ └─ python_abi 3.6.* *_cp36m, which conflicts with any installable versions previously reported;
├─ zstandard [0.14.0|0.14.1|0.15.1|0.15.2] would require
│ └─ python_abi 3.6 *_pypy36_pp73, which conflicts with any installable versions previously reported;
├─ zstandard [0.14.0|0.14.1|...|0.18.0] would require
│ └─ python_abi 3.7.* *_cp37m, which conflicts with any installable versions previously reported;
├─ zstandard [0.14.0|0.14.1|...|0.23.0] would require
│ └─ python_abi 3.8.* *_cp38, which conflicts with any installable versions previously reported;
├─ zstandard [0.14.0|0.14.1|...|0.23.0] would require
│ └─ python_abi 3.9.* *_cp39, which conflicts with any installable versions previously reported;
├─ zstandard [0.14.1|0.15.1|0.15.2|0.16.0|0.17.0] would require
│ └─ python_abi 3.7 *_pypy37_pp[73](https://github.com/openforcefield/openff-toolkit/actions/runs/10685152619/job/29617269321#step:4:76), which conflicts with any installable versions previously reported;
├─ zstandard [0.16.0|0.17.0|...|0.23.0] would require
│ └─ python_abi 3.10.* *_cp310, which conflicts with any installable versions previously reported;
├─ zstandard [0.17.0|0.18.0|0.19.0] would require
│ └─ python_abi 3.8 *_pypy38_pp73, which conflicts with any installable versions previously reported;
├─ zstandard [0.17.0|0.18.0|0.19.0] would require
│ └─ python_abi 3.9 *_pypy39_pp73, which conflicts with any installable versions previously reported;
├─ zstandard [0.18.0|0.19.0|0.21.0|0.22.0|0.23.0] would require
│ └─ python_abi 3.11.* *_cp311, which conflicts with any installable versions previously reported;
├─ zstandard 0.8.1 would require
│ └─ python 3.5* , which conflicts with any installable versions previously reported;
├─ zstandard 0.8.1 would require
│ └─ python 3.6* , which conflicts with any installable versions previously reported;
├─ zstandard 0.19.0 would require
│ └─ pypy3.8 >=7.3.11 , which conflicts with any installable versions previously reported;
├─ zstandard 0.19.0 would require
│ └─ pypy3.9 >=7.3.11 , which conflicts with any installable versions previously reported;
├─ zstandard [0.21.0|0.22.0|0.23.0] conflicts with any installable versions previously reported;
├─ zstandard 0.21.0 would require
│ └─ pypy3.9 >=7.3.12 , which conflicts with any installable versions previously reported;
├─ zstandard 0.22.0 would require
│ └─ pypy3.9 >=7.3.13 , which conflicts with any installable versions previously reported;
├─ zstandard [0.22.0|0.23.0] would require
│ └─ pypy3.9 >=7.3.15 , which conflicts with any installable versions previously reported;
└─ zstandard 0.23.0 would require
└─ python >=3.13.0rc1,<3.14.0a0 , which cannot be installed (as previously explained).
critical libmamba Could not solve for environment specs There are no Python builds for |
Maybe this could be worked around by allowing an older version and setting the environment variable, but that's a huge headache |
We're seeing failures in this deployment testing - but NOT in CI testing - because we pin |
I'm kicking tests now that we have an OFFTK package without the zstandard constraint on the |
Env built easy enough, but we've still got the import issue with OpenEye 2024.1.1
I'll keep thinking about how to resolve. |
Updates for Interchange 0.4.0/Pydantic v2 Update BRD4 notebook cosmetic commit to kick CI Revert
717f315
to
4c132f6
Compare
The tests broken by 0.4.0 have been updated. The code changes needed by the toolkit are pretty minimal. This uncovered conda-forge/openff-toolkit-feedstock#92 which we were unaware of because the relevant combinations of upstreams was not tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes broadly look good - I'm surprised so few were needed!
This is basically approved except that I think it would break everything if deployed to main channels before the Interchange 0.4 release. So my current thinking is that this should stay as a PR for now and be merged+released simultaneously with Interchange 0.4. Does that square with your understanding?
@@ -1412,9 +1412,11 @@ def get_partial_charges(self, molecule: "Molecule", **kwargs: Any) -> Quantity: | |||
c.m | |||
for c in Interchange.from_smirnoff( | |||
force_field=self, topology=[molecule], **kwargs | |||
)["Electrostatics"].charges.values() | |||
)["Electrostatics"] | |||
._get_charges() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not blocking) Is there still a public API point for getting the charges? Accessing the private API is a bit brittle so I'd prefer a public route if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into why this change was necessary; it shouldn't have changed.
A try
/except
should suffice as a compatibility workaround in the interim if you're open to that
- openeye | ||
- conda-forge | ||
dependencies: | ||
# Base depends | ||
- python | ||
- pydantic =1 | ||
- pydantic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both packaging and testing yamls, should we set this to pydantic >2
? Or is it redundant since it will be impossible to install this with a pydantic-1-compatible Interchange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think
pydantic >2
/pydantic >=2
would be unwise since that bets on current builds being compatible with Pydantic v3+ out of the box, which I don't trust.pydantic =2
should be fine, if a little redundant since the toolkit itself has no direct opinions on Pydantic compatibility - Yes, installing v1 (install-time/packaging level) will be impossible with Interchange 0.4
Updates for Interchange 0.4.0/Pydantic v2 Update BRD4 notebook cosmetic commit to kick CI Revert
…nto test-interchange-0.4.0beta1
No description provided.